-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove wrong shim thickness and typo for top back UChannel #1001
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked out this code and made comparisons between the 2016 and 2019 detectors and confirmed this does indeed fix the issue of getting different detectors with the same survey data.
@@ -556,7 +556,7 @@ public static class ModuleL3Bot extends ModuleL13Bot { | |||
//2019 MRSolt survey | |||
//protected final static double L3_new_vertical_shift = 0.7 - 0.012; | |||
//2021 nominal | |||
protected final static double L3_new_vertical_shift = 0.7; | |||
protected final static double L3_new_vertical_shift = 0.; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any clue why Matt Added this 0.7mm. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My best guess is this was to account for the shims added wrt the engineering run detector. I don't think it was that thick, will confirm with Tim. Also, this isn't even used if you supply survey data for the modules.
@@ -51,7 +51,7 @@ public void build() { | |||
AlignmentCorrection supBotCorrBack = getUChannelCorrection(false,90); | |||
supBotCorrBack.setNode(node); | |||
AlignmentCorrection supTopCorrBack = getUChannelCorrection(true,90); | |||
supTopCorr.setNode(node); | |||
supTopCorrBack.setNode(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed a bug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to properly update detector lcdd files.
eaee4b2
to
a00570d
Compare
Please hold off on merging this into master until we can discuss this at the software meeting. I would much prefer that we maintain backwards-compatibility with four years of simulation and reconstruction work. Changing the geometry in this way will invalidate those detectors. We should modify the compact.xml files to be commensurate with the new code such that the output geometry is unchanged. We should then proceed from here on out with new detectors that correct the mistake. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have not done much simulation/reconstruction work in the past 4 years. If we need to rereco some MC for some reason we can always use the 5.1 release to have the same detectors. Making things "backwards compatible" to be able to keep a bug around is poor software practice.
Removes 700um shift that Cam notices during his Survey Constant analysis and fixes bug in geometry code. The bug was as follows: by not setting the node for the top back alignment corrections, the corresponding survey volume had no access to the survey constants defined in the
compact.xml
. Fixing the typo solves this problem.